Fixes #37946 - Add 'other' option for package type in errata filters #11188

qcjames53 · 2024-10-23T21:08:09Z

What are the changes introduced in this pull request?

Added "other" option for package type in errata filters by date. This allows users to include the "newpackage" type (and others) when filtering by date and/or type. Apologies for how messy this is, more changes are to come.

Considerations taken when implementing this change?

There are a few places in the source where I wasn't quite sure if the 'other' category made sense to add. There are a lot of UI files where this functionality will be added in a future PR, but I'd like input on whether these areas (or any other) would benefit from the addition of the 'other' option:

app/mailers/katello/errata_mailer.rb
app/models/katello/host_collection.rb
app/models/katello/host/content_facet.rb

What are the testing steps for this pull request?

Run these steps before pulling this PR:

Install EPEL 8 on your foreman server:
- Create a content credential GPG key from this URL: https://dl.fedoraproject.org/pub/epel/RPM-GPG-KEY-EPEL-8.
- Create a product 'EPEL8' using the GPG key.
- 'New Repository' named 'EPEL8 x86_64' or something similar of type 'yum'. Restrict to x86_64 and set the upstream URL to https://dl.fedoraproject.org/pub/epel/8/Everything/x86_64/. Set the download policy to 'On Demand'. You may have to set the GPG key here as well.
- Run a sync on the repo.
Create a test content view and add EPEL8 to the repositories list. Publish a new version.
Add a filter to the CV, 'Errata by Date Range', include. Set the end date of the filter to today, leaving everything else blank / the same as default.
Add another filter, 'RPM', and check 'Include all RPMs not associated with any errata'.
Publish a new CV version (this will take a while).
Go to the content view version overview page and take note of the number of packages and errata. Both the package and errata count will have dropped substantially, even though nothing should be filtered.

Run these steps after pulling the PR:

Publish a new CV version. The counts should not change.
Edit the 'Errata by Date Range' filter and check the 'Other' field to allow for non-standard errata to be added to the filter. Save and publish a new CV version.
Go to the content view version overview page. The package and errata count should match the initial counts for V1.

jeremylenz · 2024-10-30T20:08:26Z

app/models/katello/erratum.rb

+    OTHER = ["other"].freeze
+    TYPES = [SECURITY, BUGZILLA, ENHANCEMENT, OTHER].flatten.freeze


My understanding is that EPEL errata can have strings in their metadata that identify the type as 'unspecified' or 'newpackage'. (see, for example, https://community.theforeman.org/t/cannot-create-errata-filter-for-unspecified-type/38981) If you intend this new "Other" type to be a catch-all, should probably handle that somehow.

Yeah exactly, 'other' is a catch-all. content_view_erratum_filter.rb is handling this at the moment (albeit in a rather gross way which sorely needs re-written).

Reopening this.. On second reading I'm still feeling uneasy about adding OTHER as a type here. Security, bugfix, and enhancement are all strings that will appear verbatim in an incoming erratum's metadata. However, "other" is a special signifier that we're using to mean "not one of these strings."

Since we store the errata type in our database, it feels wrong to me to be storing the string "other" when that's not consistent with how we treat the "valid" errata types.

My suggestion is to allow "other" as an input (user input, API param, etc.), but avoid storing the verbatim string "other" on errata objects. (Unless, of course, that's the next wacky errata type that EPEL decides to issue.) What do you think?

jeremylenz · 2024-10-30T20:26:00Z

Needs further investigation into ways this new field breaks errata management; an expert opinion is kindly requested.

A couple UI places I can think of that will need updating:

the pie chart on the Errata card in host details page
the Type column on the Content > Errata tab in host details page
the filter dropdowns on the Content > Errata tab in host details page
Content > Errata main page, maybe?

jeremylenz · 2024-10-30T20:26:51Z

should also make sure this works with scoped search - I can search for "type = security" and get all security errata; I should be able to do "type = other" and get all "newpackages" and whatever else. :D

jeremylenz · 2024-11-14T17:17:49Z

webpack/scenes/ContentViews/Details/Filters/CVErrataDateFilterContent.js

@@ -171,6 +171,15 @@ const CVErrataDateFilterContent = ({
                      {__('Bugfix')}
                    </p>
                  </SelectOption>
+                  <SelectOption
+                    isDisabled={!hasPermission(permissions, 'edit_content_views')}


If a user can't do something due to permissions, it should be hidden, not disabled.

I was following the standard for the other three options in this menu which also disable on lack of permissions (or singleselection which wasn't applicable). I think it makes sense to either keep it disabled or change all the options to hidden. Which would you prefer?

Ah, I didn't see the others. I think the general rule should still apply, and we should change all of them to hidden. But I will defer to @sjha4

I think part of it is because we have view and edit permissions on CVs and it's an editable form on the UI..So we allow view but not edit..We can rework that behavior but you can follow the existing pattern for the scope of this PR.

That sounds good to me!

jeremylenz

Tested in the UI by creating a Katello::ContentFacetErratum which made a fake applicable erratum (it didn't last long). On the Host details > Content > Errata tab, the Errata Type column is blank for "other" errata. Also there's no filter dropdown option. Will definitely need to address that in the UI followup.

I do see the "Other" option available to select for Errata by Date filters. Can we also add it to Errata (not by date) filters? Or is that outside scope here?

I did publish a filtered CV version with EPEL "Other" errata filtered out, and it appears to work as expected.

jeremylenz · 2024-11-18T17:58:08Z

app/models/katello/erratum.rb

+    OTHER = ["other"].freeze
+    TYPES = [SECURITY, BUGZILLA, ENHANCEMENT, OTHER].flatten.freeze


Reopening this.. On second reading I'm still feeling uneasy about adding OTHER as a type here. Security, bugfix, and enhancement are all strings that will appear verbatim in an incoming erratum's metadata. However, "other" is a special signifier that we're using to mean "not one of these strings."

Since we store the errata type in our database, it feels wrong to me to be storing the string "other" when that's not consistent with how we treat the "valid" errata types.

My suggestion is to allow "other" as an input (user input, API param, etc.), but avoid storing the verbatim string "other" on errata objects. (Unless, of course, that's the next wacky errata type that EPEL decides to issue.) What do you think?

jeremylenz · 2024-11-18T17:58:59Z

app/models/katello/erratum.rb

@@ -55,6 +56,7 @@ def self.of_type(type)
    scope :security, -> { of_type(Erratum::SECURITY) }
    scope :bugfix, -> { of_type(Erratum::BUGZILLA) }
    scope :enhancement, -> { of_type(Erratum::ENHANCEMENT) }
+    scope :other, -> { of_type(Erratum::OTHER) }


My comment above will slightly complicate things like this, but I think it's worth it.

jeremylenz · 2024-11-18T18:08:01Z

app/models/katello/content_view_erratum_filter.rb

+      valid_types = Erratum::TYPES.reject { |type| type == "other" }
+      conditions << errata_types_in(types.reject { |type| type == "other" })
+      conditions << errata_types_not_in(valid_types) if types.include?("other")
+      conditions.reduce(nil) do |combined_clause, condition|
+        combined_clause ? combined_clause.or(condition) : condition
+      end


I think I understand what this is doing but it's pretty hairy. How can we make it clearer? My issues with it are

Erratum::TYPES.reject { |type| type == "other" } is too similar to types.reject { |type| type == "other" } and that's confusing. Took me a minute to realize that the former is a slice of the constant, and the latter is a slice of the (user?) input.

The single-line if types.include?("other") would be more readable as a traditional if statement. That way you may even include a comment explaining what this is doing.

Can we please get some comments explaining what each line is doing?

This will require a bit of thought. I tried to whip up a quick solution to post as a suggestion here, but one wasn't immediately obvious so this comment is just complaints now, sorry!

I can absolutely explain this better. Removing the 'other' type would make this section so much clearer anyways. Discussion below...

sjha4 · 2024-11-18T19:25:32Z

Did this not need any change in the repo indexing logic from pulp to actually save the errata with type: other?

jeremylenz · 2024-11-18T19:28:44Z

Did this not need any change in the repo indexing logic from pulp to actually save the errata with type: other?

It might, but perhaps our comments above crossed. I don't think we should save them with that type.

qcjames53 · 2024-11-18T20:28:33Z

@jeremylenz :

My suggestion is to allow "other" as an input (user input, API param, etc.), but avoid storing the verbatim string "other" on errata objects. (Unless, of course, that's the next wacky errata type that EPEL decides to issue.) What do you think?

I'm a fan of that idea but it's going to lead to a bit of a compromise to solve the bug this PR was originally addressing (non-bugfix, enhancement, security types being removed with errata filter). When the errata filter has to filter out types, would you be okay letting everything through if all three supported types are checked?

qcjames53 · 2024-11-18T20:29:20Z

There's not really an API way to allow for 'other' types to be included when an errata filter runs since no API calls are made.

jeremylenz · 2024-11-18T20:48:32Z

When the errata filter has to filter out types, would you be okay letting everything through if all three supported types are checked?

Are you proposing removing "Other" from the filter creation options as well? I didn't mean that. My suggestion is that the UI remains as-is, but "other" should not be a member of the TYPES constant and should not be stored in the database, that's all.

qcjames53 · 2024-11-18T21:10:41Z

Jeremy and I chatted outside of Github and came to the conclusion it would be best to keep the UI as it currently is but change the backend such that the 'other' checkbox changes a new boolean field in the errata filter model. The 'other' type as it currently exists is to be removed from all places it is referenced.

qcjames53 · 2024-11-21T17:30:32Z

I just pushed my current changes. I haven't finished fixing the forms yet but I wanted to know your thoughts on the new field. How are things looking?

jeremylenz · 2024-11-21T17:49:14Z

Looking much better! heading in the right direction. 👍

qcjames53 · 2024-11-21T22:27:31Z

@jeremylenz I pushed what I have for UI changes so far. There are two issues I'm unfortunately struggling with:

The state of ruleAllowOtherTypes doesn't seem to be updating the database entry for the filter when changed. Everything seems to be plugged together correctly so I think I'm just missing something obvious.
The 'other' checkbox is not currently driven by the state of ruleAllowOtherTypes/allowOtherTypes. I don't really have a strategy to address this issue at the moment and was wondering if you'd mind taking a look since you are a lot more proficient with React than I am.

qcjames53 · 2024-11-21T22:30:50Z

Also I was looking at this and there are some obvious issues with single selection for the 'other' checkbox as well. I'd love to just have a separate checkbox in the existing list that links 1:1 with the status for allowOtherTypes but I'm not sure that's something I can do inside of the patternfly selection widget. Is that an option? Single selection logic would need to be re-written in that case but it would make the other parts much easier.

jeremylenz · 2024-11-22T20:37:45Z

webpack/scenes/ContentViews/Details/Filters/CVErrataDateFilterContent.js

+    let updatedSelectedTypes;
    if (selectedTypes.includes(selection)) {
+      // If the selection is the only selection remaining, do not allow it to be removed
      if (selectedTypes.length === 1) return;
-      setSelectedTypes(selectedTypes.filter(e => e !== selection));
-    } else setSelectedTypes([...selectedTypes, selection]);
+
+      // Filter out the current selection from the selected types to update
+      updatedSelectedTypes = selectedTypes.filter(e => e !== selection);
+    } else {
+      updatedSelectedTypes = [...selectedTypes, selection];
+    }
+
+    // If 'other' is selected, update the allowOtherTypes field
+    setSelectedTypes(updatedSelectedTypes.filter(e => e !== 'other'));
+    setAllowOtherTypes(updatedSelectedTypes.includes('other'));


The filter in line 103 is what's causing the weird behavior of the checkboxes. But the good news is, there's no need for that filter. Let me explain..

Remember that in React, state stays inside the component. Just because you have a state called allowOtherTypes doesn't mean it will magically be bound to some data field and updated when it changes. (That's closer to how AngularJS behaves.) All state does is cause its component to rerender when its value is updated.

I see these two states here:

const [selectedTypes, setSelectedTypes] = useState(types); const [allowOtherTypes, setAllowOtherTypes] = useState(ruleAllowOtherTypes);

State describes what's happening currently within the component. I don't think we need the allowOtherTypes state at all. "Other" can just be a selected type just like any other.

Where we need to handle it is above, in the onSave method. In there, we can normalize the data, e.g. see if selectedTypes include "other" and then transform that into the allow_other_types param -- but only when we actually send the data to the backend. Before then, it doesn't matter.

So my suggestion is

Revert all the refactoring from the onTypeSelect method - it can go back to what it was doing before.

In the onSave method, see if selectedTypes includes other. If it does, remove it from selectedTypes and instead send the allow_other_types param. You'll want to add this to the object right after types: selectedTypes,.

jeremylenz

Oh also, would be good to get the same changes in for the Errata (not by date) filters.

webpack/scenes/ContentViews/Details/Filters/CVErrataDateFilterContent.js

qcjames53 · 2024-11-23T05:32:46Z

I've just pushed a working UI for the errata filter by date page. I'm using this to see if any unit tests fail for that webpage. There are additional changes needed for the errata filter by id page to function, detailed below:

Current progress:

API calls work for fetch and update of errata filters with the new allow_other_types parameter.
The errata filters by date page works with the new 'other' checkbox and allows the user to save filters by date with the new field.
The errata filters by id page contains an 'other' checkbox and passes the field to the API but app/controllers/katello/concerns/api/v2/repository_content_controller.rb and app/controllers/katello/api/v2/errata_controller.rb need modifications to be able to understand the changing input.
Added new unit tests for errata filters with allow_other_types.

pr-processor bot added the Not yet reviewed label Oct 23, 2024

jeremylenz reviewed Oct 30, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 30, 2024

qcjames53 force-pushed the 37946 branch 2 times, most recently from 5bd7a8a to e432bb1 Compare November 11, 2024 19:51

qcjames53 marked this pull request as ready for review November 11, 2024 19:55

qcjames53 force-pushed the 37946 branch from e432bb1 to 0732029 Compare November 11, 2024 21:09

sjha4 self-requested a review November 12, 2024 20:53

jeremylenz reviewed Nov 14, 2024

View reviewed changes

jeremylenz reviewed Nov 18, 2024

View reviewed changes

qcjames53 force-pushed the 37946 branch 2 times, most recently from 6202076 to 250befb Compare November 21, 2024 17:29

qcjames53 force-pushed the 37946 branch 2 times, most recently from 22b2fe7 to ac6cace Compare November 21, 2024 22:21

jeremylenz reviewed Nov 22, 2024

View reviewed changes

qcjames53 force-pushed the 37946 branch from ac6cace to 020a2bd Compare November 22, 2024 21:58

jeremylenz reviewed Nov 22, 2024

View reviewed changes

qcjames53 force-pushed the 37946 branch from 020a2bd to f0d16e3 Compare November 23, 2024 05:23

Fixes #37946 - Add 'allow_other_types' option in errata filters

47db3d2

qcjames53 force-pushed the 37946 branch from f0d16e3 to 47db3d2 Compare November 23, 2024 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37946 - Add 'other' option for package type in errata filters #11188

Fixes #37946 - Add 'other' option for package type in errata filters #11188

qcjames53 commented Oct 23, 2024 •

edited

Loading

jeremylenz Oct 30, 2024

qcjames53 Oct 30, 2024

jeremylenz Nov 18, 2024

jeremylenz commented Oct 30, 2024

jeremylenz commented Oct 30, 2024

jeremylenz Nov 14, 2024

qcjames53 Nov 14, 2024

jeremylenz Nov 14, 2024

sjha4 Nov 18, 2024

qcjames53 Nov 18, 2024

jeremylenz left a comment

jeremylenz Nov 18, 2024

jeremylenz Nov 18, 2024

jeremylenz Nov 18, 2024

qcjames53 Nov 18, 2024

sjha4 commented Nov 18, 2024

jeremylenz commented Nov 18, 2024

qcjames53 commented Nov 18, 2024

qcjames53 commented Nov 18, 2024

jeremylenz commented Nov 18, 2024

qcjames53 commented Nov 18, 2024 •

edited

Loading

qcjames53 commented Nov 21, 2024

jeremylenz commented Nov 21, 2024

qcjames53 commented Nov 21, 2024

qcjames53 commented Nov 21, 2024

jeremylenz Nov 22, 2024

jeremylenz left a comment

qcjames53 commented Nov 23, 2024

		OTHER = ["other"].freeze
		TYPES = [SECURITY, BUGZILLA, ENHANCEMENT, OTHER].flatten.freeze

Fixes #37946 - Add 'other' option for package type in errata filters #11188

Are you sure you want to change the base?

Fixes #37946 - Add 'other' option for package type in errata filters #11188

Conversation

qcjames53 commented Oct 23, 2024 • edited Loading

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz commented Oct 30, 2024

jeremylenz commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjha4 commented Nov 18, 2024

jeremylenz commented Nov 18, 2024

qcjames53 commented Nov 18, 2024

qcjames53 commented Nov 18, 2024

jeremylenz commented Nov 18, 2024

qcjames53 commented Nov 18, 2024 • edited Loading

qcjames53 commented Nov 21, 2024

jeremylenz commented Nov 21, 2024

qcjames53 commented Nov 21, 2024

qcjames53 commented Nov 21, 2024

Choose a reason for hiding this comment

jeremylenz left a comment

Choose a reason for hiding this comment

qcjames53 commented Nov 23, 2024

qcjames53 commented Oct 23, 2024 •

edited

Loading

qcjames53 commented Nov 18, 2024 •

edited

Loading